Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[16881] Secure DS test suite #62

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

Mario-DL
Copy link
Member

This PR intends to add the Secure Discovery Server (with all security protections enabled a.k.a Sv2.1_SP;Cv2.1_SP) Test Suite, which basically runs all the existing test-case scenarios with a new properties.xml file in which properties (including security ones) can be defined.

This properties.xml file extends the previously defined properties in each XML test-case scenario.

@MiguelCompany MiguelCompany changed the title Secure DS test suite [16881] Secure DS test suite Feb 17, 2023
@MiguelCompany
Copy link
Member

Tests running here

@MiguelCompany
Copy link
Member

New run here

}
else
{
ret = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could be a little more specific on what the failure is with a LOG_ERROR (missing name/value)

Comment on lines 26 to 43
<topic_rule>
<topic_expression>*</topic_expression>
<enable_discovery_protection>true</enable_discovery_protection>
<enable_liveliness_protection>true</enable_liveliness_protection>
<enable_read_access_control>true</enable_read_access_control>
<enable_write_access_control>true</enable_write_access_control>
<metadata_protection_kind>ENCRYPT</metadata_protection_kind>
<data_protection_kind>ENCRYPT</data_protection_kind>
</topic_rule>
<topic_rule>
<topic_expression>topic*</topic_expression>
<enable_discovery_protection>true</enable_discovery_protection>
<enable_liveliness_protection>true</enable_liveliness_protection>
<enable_read_access_control>false</enable_read_access_control>
<enable_write_access_control>false</enable_write_access_control>
<metadata_protection_kind>ENCRYPT</metadata_protection_kind>
<data_protection_kind>ENCRYPT</data_protection_kind>
</topic_rule>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wha's the rationale behind this distinction? Why disable access control for reading and writing on topic* ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second topic rule makes no sense. The idea is to have all the protections enabled for all topics

test/run_test.py Outdated
props_file = open(config_params['properties']['SECURITY'], "r+")
data = props_file.read()
#Replace all occurrences of the required label
data = data.replace('<CERTS_RELATIVE_PATH>', args.certs_path)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add some sanity check to test that the provided certs_path location is actually a valid directory before using it here.

test/run_test.py Outdated
props_file.truncate()
props_file.close()
else:
logger.error('Certs file not found at ' + args.certs_path)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a folder instead of a file


if("${TEST}" IN_LIST FAIL_TEST_CASES)
set_property(TEST ${TEST_NAME} PROPERTY LABELS xfail)
if(SECURITY)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this need a matching OPTION statement detailing the default value and what it does?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it is the first time SECURITY is included in the repo, maybe it would be a good idea

Signed-off-by: Mario Dominguez <[email protected]>
Copy link
Contributor

@jsan-rt jsan-rt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants